Sync prefs redo issue 6504 #3988

AlexeyBarabash · 2019-11-13T22:07:08Z

Submitter Checklist:

13 tests failed:
    BraveRewardsBrowserTest.AutoContribution (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2052)
    BraveRewardsBrowserTest.ContributionWithLowAmount (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2218)
    BraveRewardsBrowserTest.InsufficientNotificationForInsufficientAmount (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2533)
    BraveRewardsBrowserTest.InsufficientNotificationForVerifiedInsufficientAmount (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2568)
    BraveRewardsBrowserTest.MultipleRecurringOverBudgetAndPartialAutoContribution (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2964)
    BraveRewardsBrowserTest.PendingContributionTip (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2415)
    BraveRewardsBrowserTest.RecurringAndPartialAutoContribution (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2917)
    BraveRewardsBrowserTest.RecurringTipForUnverifiedPublisher (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2197)
    BraveRewardsBrowserTest.RecurringTipForVerifiedPublisher (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2176)
    BraveRewardsBrowserTest.TipConnectedPublisherAnonAndConnected (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2792)
    BraveRewardsBrowserTest.TipNonIntegralAmount (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2876)
    BraveRewardsBrowserTest.TipUnverifiedPublisher (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2156)
    BraveRewardsBrowserTest.TipVerifiedPublisher (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2137)
1 test timed out:
    BraveRewardsBrowserTest.ProcessPendingContributions (../../brave/components/brave_rewards/browser/rewards_service_browsertest.cc:2615)

Test Plan:

Please see the next message

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

AlexeyBarabash · 2019-11-14T18:31:07Z

Steps to check the issue in different situation

I. Check how works reset with Leave Sync Chain button

Create sync chain of 2 devices, deviceA and deviceB
Press Leave Sync Chain button on deviceA
Ensure chain have been reset on deviceA, but there is deviceB on deviceB itself.
In other words, on deviceA Sync setup initial page is displayed; and on deviceB may appear a dialog of Choose device type to connect to - because deviceB has only one device in it's chain, deviceB itself - in this case close the dialog with cross, or in version without cross press Computer and then Cancel to see the list of devices.

II. Check how works reset over pressing cross near this device

Create sync chain of 2 devices, deviceA and deviceB
Press cross link near this device
Ensure chain have been reset on deviceA, but there is deviceB on deviceB itself

III. Check how works Reset over pressing cross near opposite device

Create sync chain of 2 devices, deviceA and deviceB
From deviceB press cross link near deviceA,
Ensure chain have been reset on deviceA only

IV. Check the original STR from brave/brave-browser#6504 becomes not applicable

Prepare clear profiles deviceA and deviceB
Launch profile A, create new sync chain, ensure sync code is shown, but don't copy the code. Close deviceA.
Launch profile B, create new sync chain, copy sync code, don't close.
Launch profile A - it should show none or one device and View Sync Code button is available, as sync is configured, so STR from the original issue are not available.

V. Check this PR build works with from old version by STR at brave-browser#6504

Prepare clear profiles deviceA and deviceB
Launch Stable version 1.0 on profileA. Create new sync chain, ensure sync code is shown, but don't copy the code. Close deviceA.
Launch profile B, create new sync chain, copy sync code, don't close.
Launch this PR build on profile A - it will show there is one device in chain (and should ask to connect other devices).

VI. Check we send bookmarks as soon as sync is enabled even if there are one device in chain

Prepare clear profile deviceA, containing bookmarks A1.com and A2.com
Start a new sync chain
Open chrome://inspect/#extensions and check for Brave Sync logs.
Ensure you can see "send-sync-records" category_name="BOOKMARKS" records=...

This below will go to the a separate PR for issue https://github.com/brave/brave-browser/issues/3628

Check how works reset with Leave Sync Chain button when the device is offline

Create sync chain of 2 devices, deviceA and deviceB
Turn off the internet connection on deviceA
Press Leave Sync Chain button on deviceA
Ensure chain have been reset on deviceA, but there deviceB still have deviceA in list
Close browser deviceA; turn on the internet conenction, launch browser of deviceA - ensure there are no sync chain on deviceA

AlexeyBarabash · 2019-11-15T12:17:56Z

Created an issue for UI changes: brave/brave-browser#6941 .

AlexeyBarabash · 2019-11-15T12:20:13Z

Marked it ready for review to trigger the CI.
Two commits of this PR should be removed / replaced before merge

AlexeyBarabash · 2019-11-18T18:20:34Z

rebased due to Version mismatch between brave-browser and brave-core in package.json CI failure

components/brave_sync/brave_profile_sync_service_impl.cc

darkdh · 2019-11-18T20:14:57Z

components/brave_sync/brave_profile_sync_service_impl.cc

+          device_name, object_id, SyncRecord::Action::A_DELETE, device_id);
+      base::PostDelayedTask(
+          FROM_HERE, {content::BrowserThread::UI},
+          base::BindOnce(&BraveProfileSyncServiceImpl::OnResolvedPreferencesWrp,


what is this for? I thought FetchDevices will also trigger OnResolvedPreferences after we send delete this device record and then we see this device get deleted, then we reset.

This is for the case when we want to reset the sync but internet connection is offline.
When the internet is offline, FetchDevices will get empty result, and we will not be able to call ResetSyncInternal()

Actually we can simplify it by making sure DELETE device record is sent by OnRecordsSent and then we can call ResetSyncInternal() without involving OnResolvedPreferences

This delayed task doesn't really solve this situation

IV. Check how works reset with Leave Sync Chain button when the device is offline 1. Create sync chain of 2 devices, deviceA and deviceB 2. Turn off the internet connection on deviceA 3. Press Leave Sync Chain button on deviceA 4. Ensure chain have been reset on deviceA, but there deviceB still have deviceA in list 5. Close browser deviceA; turn on the internet conenction, launch browser of deviceA - ensure there are no sync chain on deviceA

device B still doesn't get delete record from A

Here are what we can do

If we want to make sure device B gets DELETE A record, we need to block A from Reset until device A sends the record (which means device A get online)

Or we can leverage OnRecordsSent as I mentioned in https://github.com/brave/brave-core/pull/3988/files#r348770951
It will get called no matter success or failure, then we call ResetSyncInternal() (failure will have empty records)

For the case when internet connection is off there could not be a perfect solution, because we cannot send device DELETE record.

The option with delayed task gives two good points:

If internet connection is on, then the device is able to send DELETE record, so it informs all other. No disadvantages here.

If internet connection is off, then the device is able to make a proper self-reset. The only disadvantage is - other devices would not be informed and could see zobmie.

Any block of ourselves until get device DELETE record confirmation, makes sync to be under pending reset for an unknown time.

As for now, when the internet connection is off, sending DEVICE delete record will never reach to AWS, because we do resend for bookmarks only.

We can leverage OnRecordsSent and do there a cancellation of delayed task if we see device DELETE with our device_id. @darkdh what do you think?

We don’t need delayed task if we use OnRecordsSent because when offline or any failed to send, it will get called with EMPTY records to indicate failure.

As per Slack discussion, the case of reset when the device is offline will be done as separate issue (brave/brave-browser#3628) + separate PR after this PR will be merged.

components/brave_sync/brave_profile_sync_service_impl.cc

components/brave_sync/brave_sync_service_unittest.cc

components/brave_sync/ui/components/modals/viewSyncCode.tsx

AlexeyBarabash · 2019-11-20T11:59:33Z

@darkdh addressed all comments, could you please look again?

darkdh

There are some UI issues like

Right after sync chain is created, copy sync code/scan QR code dialog dismiss immediately and I have to click
in order to add second device
copy sync code/scan QR code can not be dismissed when there is only one device in sync chain( only achievable by close the tab or refresh the brave://sync), otherwise, it leads to Reset

Both can be addressed by brave/brave-browser#6941 (comment)

darkdh · 2019-11-20T23:21:34Z

components/brave_sync/brave_profile_sync_service_impl.cc

+          device_name, object_id, SyncRecord::Action::A_DELETE, device_id);
+      base::PostDelayedTask(
+          FROM_HERE, {content::BrowserThread::UI},
+          base::BindOnce(&BraveProfileSyncServiceImpl::OnResolvedPreferencesWrp,


This delayed task doesn't really solve this situation

IV. Check how works reset with Leave Sync Chain button when the device is offline 1. Create sync chain of 2 devices, deviceA and deviceB 2. Turn off the internet connection on deviceA 3. Press Leave Sync Chain button on deviceA 4. Ensure chain have been reset on deviceA, but there deviceB still have deviceA in list 5. Close browser deviceA; turn on the internet conenction, launch browser of deviceA - ensure there are no sync chain on deviceA

device B still doesn't get delete record from A

Here are what we can do

If we want to make sure device B gets DELETE A record, we need to block A from Reset until device A sends the record (which means device A get online)

Or we can leverage OnRecordsSent as I mentioned in https://github.com/brave/brave-core/pull/3988/files#r348770951
It will get called no matter success or failure, then we call ResetSyncInternal() (failure will have empty records)

AlexeyBarabash · 2019-11-22T11:22:34Z

Squashed all fix-meaning commits

components/brave_sync/brave_profile_sync_service_impl.cc

AlexeyBarabash · 2019-11-25T10:53:17Z

CI failed with two errors:

19:51:06  ERROR at //services/media_session/public/mojom/BUILD.gn:27:25: Assignment had no effect.
19:51:06    export_header_blink = "third_party/blink/public/platform/web_common.h"
19:51:06                          ^-----------------------------------------------
19:51:06  You set the variable "export_header_blink" here and it was unused before it went
19:51:06  out of scope.
19:51:06  See //content/public/browser/BUILD.gn:408:5: which caused the file to be included.
19:51:06      "//services/media_session/public/mojom",
19:51:06      ^--------------------------------------

and an unclear at npm run create_dist -- Release --channel=nightly --official_build=true

IOError: [Errno 2] No such file or directory: 'Brave Browser Nightly.app/Contents/Frameworks/Brave Browser Nightly Framework.framework/Versions/78.1.3.24/Brave Browser Nightly Framework'

both are not related with PR, doing last attempt rebase.

Allow one device to be in chain. Remove unnecessary states.

…UI PR

AlexeyBarabash · 2019-11-25T14:18:41Z

CI failed with only error


13:28:12  ERROR at //services/media_session/public/mojom/BUILD.gn:27:25: Assignment had no effect.
13:28:12    export_header_blink = "third_party/blink/public/platform/web_common.h"
13:28:12                          ^-----------------------------------------------
13:28:12  You set the variable "export_header_blink" here and it was unused before it went
13:28:12  out of scope.
13:28:12  See //content/public/browser/BUILD.gn:408:5: which caused the file to be included.
13:28:12      "//services/media_session/public/mojom",
13:28:12      ^--------------------------------------

for npm run build -- Release --channel=nightly --official_build=true --target_os=ios

kjozwiak · 2019-12-17T07:32:06Z

@AlexeyBarabash should this PR be in 1.3.x. Currently doesn't have an associated milestone.

AlexeyBarabash · 2019-12-17T12:48:30Z

@kjozwiak yes, it should be in 1.3.x, as it was merged when master was 1.3.x . Added the milestone.

AlexeyBarabash mentioned this pull request Nov 13, 2019

Sync reset if browser was closed before the chain was fully created #3730

Closed

32 tasks

AlexeyBarabash force-pushed the sync_prefs_redo_issue_6504 branch 5 times, most recently from c6832bc to 9cda93a Compare November 14, 2019 18:19

AlexeyBarabash mentioned this pull request Nov 15, 2019

Modify Brave Sync UI to allow one device in chain brave/brave-browser#6941

Closed

AlexeyBarabash self-assigned this Nov 15, 2019

AlexeyBarabash added enhancement feature/sync labels Nov 15, 2019

AlexeyBarabash requested review from darkdh, cezaraugusto, mkarolin and bridiver November 15, 2019 12:18

AlexeyBarabash marked this pull request as ready for review November 15, 2019 12:19

AlexeyBarabash force-pushed the sync_prefs_redo_issue_6504 branch from 9cda93a to 5b3dbb1 Compare November 18, 2019 18:19

darkdh reviewed Nov 18, 2019

View reviewed changes

AlexeyBarabash force-pushed the sync_prefs_redo_issue_6504 branch 3 times, most recently from 50f7596 to 6f8b187 Compare November 20, 2019 11:54

AlexeyBarabash requested a review from darkdh November 20, 2019 11:59

darkdh reviewed Nov 20, 2019

View reviewed changes

AlexeyBarabash force-pushed the sync_prefs_redo_issue_6504 branch 3 times, most recently from 16f6653 to 209f9ce Compare November 22, 2019 11:12

AlexeyBarabash requested a review from darkdh November 22, 2019 14:04

bridiver reviewed Nov 22, 2019

View reviewed changes

components/brave_sync/brave_profile_sync_service_impl.cc Outdated Show resolved Hide resolved

AlexeyBarabash force-pushed the sync_prefs_redo_issue_6504 branch from 209f9ce to 00ba1bd Compare November 22, 2019 17:41

darkdh approved these changes Nov 22, 2019

View reviewed changes

AlexeyBarabash added 5 commits November 25, 2019 13:13

Redo Brave sync prefs

7a4d18c

Allow one device to be in chain. Remove unnecessary states.

Workaround for empty passphrase - will be reverted after UI PR

a3708ba

Updated testcases for redone Brave sync prefs

d4b5b8b

Remove obsolete pref of sync prev seed

61d4ba5

Changes in sync UI to allow test of service - will be reverted after …

47c29b6

…UI PR

AlexeyBarabash force-pushed the sync_prefs_redo_issue_6504 branch from 00ba1bd to 47c29b6 Compare November 25, 2019 11:16

bridiver approved these changes Nov 25, 2019

View reviewed changes

AlexeyBarabash merged commit b226231 into master Nov 25, 2019

AlexeyBarabash deleted the sync_prefs_redo_issue_6504 branch November 25, 2019 16:46

AlexeyBarabash mentioned this pull request Dec 6, 2019

Sync missing bookmarks after chain creation brave/brave-browser#7251

Closed

AlexeyBarabash added this to the 1.3.x - Dev milestone Dec 17, 2019

btlechowski mentioned this pull request Feb 5, 2020

Sync cannot enable chain sometimes brave/brave-browser#6504

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync prefs redo issue 6504 #3988

Sync prefs redo issue 6504 #3988

AlexeyBarabash commented Nov 13, 2019 •

edited

Loading

AlexeyBarabash commented Nov 14, 2019 •

edited

Loading

AlexeyBarabash commented Nov 15, 2019

AlexeyBarabash commented Nov 15, 2019

AlexeyBarabash commented Nov 18, 2019

darkdh Nov 18, 2019

AlexeyBarabash Nov 19, 2019

darkdh Nov 20, 2019

darkdh Nov 20, 2019

AlexeyBarabash Nov 21, 2019

darkdh Nov 21, 2019

AlexeyBarabash Nov 22, 2019 •

edited

Loading

AlexeyBarabash commented Nov 20, 2019

darkdh left a comment

darkdh Nov 20, 2019

AlexeyBarabash commented Nov 22, 2019

AlexeyBarabash commented Nov 25, 2019

AlexeyBarabash commented Nov 25, 2019

kjozwiak commented Dec 17, 2019

AlexeyBarabash commented Dec 17, 2019

Sync prefs redo issue 6504 #3988

Sync prefs redo issue 6504 #3988

Conversation

AlexeyBarabash commented Nov 13, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

AlexeyBarabash commented Nov 14, 2019 • edited Loading

AlexeyBarabash commented Nov 15, 2019

AlexeyBarabash commented Nov 15, 2019

AlexeyBarabash commented Nov 18, 2019

darkdh Nov 18, 2019

Choose a reason for hiding this comment

AlexeyBarabash Nov 19, 2019

Choose a reason for hiding this comment

darkdh Nov 20, 2019

Choose a reason for hiding this comment

darkdh Nov 20, 2019

Choose a reason for hiding this comment

AlexeyBarabash Nov 21, 2019

Choose a reason for hiding this comment

darkdh Nov 21, 2019

Choose a reason for hiding this comment

AlexeyBarabash Nov 22, 2019 • edited Loading

Choose a reason for hiding this comment

AlexeyBarabash commented Nov 20, 2019

darkdh left a comment

Choose a reason for hiding this comment

darkdh Nov 20, 2019

Choose a reason for hiding this comment

AlexeyBarabash commented Nov 22, 2019

AlexeyBarabash commented Nov 25, 2019

AlexeyBarabash commented Nov 25, 2019

kjozwiak commented Dec 17, 2019

AlexeyBarabash commented Dec 17, 2019

AlexeyBarabash commented Nov 13, 2019 •

edited

Loading

AlexeyBarabash commented Nov 14, 2019 •

edited

Loading

AlexeyBarabash Nov 22, 2019 •

edited

Loading